fix(hooks): question-preference-hook must not emit permissionDecision 'defer' (merges #2165 + #1924)#2318
Open
Akagilnc wants to merge 4 commits into
Open
Conversation
…erence-hook Silent pass-through (exit 0, optional additionalContext only) instead of explicit permissionDecision: 'defer'. Claude Code treats defer as a real pause/resume decision and honors it in print/non-interactive mode, which swallowed every AskUserQuestion (no tool_result, resume replay loops). Fixes the root cause behind garrytan#2006/garrytan#2035/garrytan#2207/garrytan#2292/garrytan#2310. Merges PR garrytan#2165 (print-mode why comment + foundation) with PR garrytan#1924 (passThrough rename, accurate docs, stricter empty-stdout assertions). deny paths (never-ask auto-decide, Conductor prose redirect) unchanged. Co-authored-by: dpnascimento <dpnascimento@users.noreply.github.com> Co-authored-by: jacksondc <jacksondc@users.noreply.github.com>
Round-1 fixer pass against review findings on fix/auq-hook-neutral-passthrough (4c082806). Tests: - empty-stdin case uses expectNoDecision (strict empty stdout; drops dead JSON.parse assertions that were always true when stdout was empty) - add MCP-named AUQ + no preference → expectNoDecision (real failure shape for garrytan#2006/garrytan#2035/garrytan#2292/garrytan#2310) Docs (docs/spikes/claude-code-hook-mutation.md): - restore multi-hook precedence fact: deny > ask > allow (most restrictive wins), keep defer-is-not-pass-through clarification - move "no output" out of permissionDecision values list - mark revised date for the pass-through correction Scope note for PR body: this PR addresses silent pass-through for the hook itself (garrytan#2006/garrytan#2035/garrytan#2292/garrytan#2310). garrytan#2207 Conductor auto-install belongs to garrytan#2208 and is not claimed fixed here. Co-authored-by: dpnascimento <dpnascimento@users.noreply.github.com> Co-authored-by: jacksondc <jacksondc@users.noreply.github.com>
Round-2 fixer pass on fix/auq-hook-neutral-passthrough. Tests: - empty-stdin crash-safety case asserts empty stdout directly (no JSON.parse of '' that could never fail after expectNoDecision) - memory additionalContext-only path also requires hookEventName PreToolUse (garrytan#2310 output shape) Docs (docs/spikes/claude-code-hook-mutation.md): - Auto-decide example matches live hook: deny + permissionDecisionReason (not allow + updatedInput); open-question item updated for deny path Co-authored-by: dpnascimento <dpnascimento@users.noreply.github.com> Co-authored-by: jacksondc <jacksondc@users.noreply.github.com>
Round-3 fixer pass on fix/auq-hook-neutral-passthrough. M1 — empty-stdin crash-safety case now asserts res.stdout directly (no ?? '' / || '' mask). Matches expectNoDecision, garrytan#1924 original, and every other empty-stdout assertion in-repo. Sh1 — spike Answer + updatedInput semantics keep the platform fact but qualify that plan-tune lives on deny+reason (Implementation examples), so the headline no longer contradicts the live-hook section. Co-authored-by: dpnascimento <dpnascimento@users.noreply.github.com> Co-authored-by: jacksondc <jacksondc@users.noreply.github.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
manhit96
added a commit
to manhit96/gstack
that referenced
this pull request
Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Picking up the handoff from #2165. @time-attack asked there on 2026-07-15:
Neither PR has moved since, so this branch does that merge. Credit belongs to @dpnascimento (#2165) and @jacksondc (#1924); every commit here carries both as co-authors. If either of them would rather carry it forward, I am happy to close this in favour of theirs.
The bug
hosts/claude/hooks/question-preference-hook.tsemittedpermissionDecision: "defer"on its no-opinion path.deferis a real Claude Code value meaning "pause this tool call and hand control back", and it is honored in print/non-interactive mode. So in any non-interactive session, everyAskUserQuestionthe hook did not auto-decide got parked and never executed. On my machine one such call was replayed several times across resumes, and notool_resultwas ever written for it.What changed
defer()becomespassThrough(): silentexit 0, orhookSpecificOutput.additionalContextalone when plan-tune has context to inject. NopermissionDecisionon that path.denypaths (never-ask auto-decide, Conductor prose redirect) are untouched.deny > ask > allowprecedence fact that Fix AskUserQuestion hook pass-through #1924 deleted outright, moves "No output" out of thepermissionDecision values:list where it is not a value, and rewrites the Auto-decide example from the never-implementedallow+updatedInputto thedeny+ reason shape the hook actually emits.expectNoDecision(from Fix AskUserQuestion hook pass-through #1924) now covers about ten pass-through cases, plus two cases Fix AskUserQuestion hook pass-through #1924 lacked. One ismcp__conductor__AskUserQuestionwith no preference, which is the exact shape the bug reports fire on. The other assertshookEventName === 'PreToolUse'on theadditionalContext-only output.Scope
Fixes the root cause behind #2006, #2035, #2292 and #2310.
#2207 is not fixed here. That one is the Conductor auto-install of the hook, owned by #2208. An early commit body on this branch listed #2207 among the root-cause issues; that wording was too broad, and this PR body is authoritative on claim surface.
Verification
Two things I could not verify
The cathedral e2e assertion never executed locally.
test/skill-e2e-plan-tune-cathedral.test.tsisEVALS=1-gated. Forcing it withEVALS_ALL=1 EVALS=1gives 0 pass / 5 fail, because the fixture scaffold does not copylib/, so the hook dies withCannot find module '../../../lib/is-conductor'atexpect(res.status).toBe(0), before the pass-through assertion runs. That fixture gap predates this branch and is not fixed here. Gate coverage for this behaviour is the two unit test files above.The Conductor deny reason carries a premise I could not confirm. In Conductor the hook still denies with a prose redirect whose reason cites the MCP variant being flaky. That premise was tied to the same
deferpath this PR removes, and I have not re-proven it under current Claude Code. The behaviour is deliberately unchanged, since it is out of scope, but that wording is now a historical claim this PR does not confirm.I can rebase or reshape this however suits the review.